Skip to content

Conversation

@Woyken
Copy link
Contributor

@Woyken Woyken commented Aug 28, 2025

Refactored a bit router options.Wrap and options.InnerWrap usage in RouterProvider and Match components.
Now they will be able to provide context for their children components.

Added few tests for scenarios that were failing before.

Fixes #3744

Summary by CodeRabbit

  • New Features

    • Consistent wrapper handling across routing so routed content and pending states share the same wrapper/context without adding extra DOM nodes.
    • Exposed createMemoryHistory for memory-based routing in apps and tests.
  • Refactor

    • Simplified suspense and wrapper handling for clearer, safer rendering behavior.
  • Tests

    • Added tests verifying context propagation through wrappers and into pending (fallback) states.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 28, 2025

Walkthrough

Refactors RouterProvider and Matches to always apply an optional wrapper (Wrap/InnerWrap or SafeFragment), inlines Suspense fallback rendering, and adds tests ensuring Wrap/InnerWrap-provided Solid context is visible in route components and pending fallbacks. No public API signature changes.

Changes

Cohort / File(s) Summary
Wrapper unification in core
packages/solid-router/src/RouterProvider.tsx, packages/solid-router/src/Matches.tsx
Use SafeFragment as a no-op default and always render an OptionalWrapper (`Wrap
Tests for Wrap/InnerWrap context propagation
packages/solid-router/tests/RouterProvider.test.tsx, packages/solid-router/tests/Matches.test.tsx
Add tests that provide a Solid context via Wrap/InnerWrap and assert the context is available inside routed components and in the defaultPendingComponent; use createMemoryHistory, sleep, and Solid context helpers.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App
  participant RouterProvider
  participant OptionalWrap as Wrap/SafeFragment
  participant RouterCtx
  participant Matches
  participant OptionalInner as InnerWrap/SafeFragment
  participant Suspense
  participant RouteComponent
  participant PendingFallback

  App->>RouterProvider: render(router, children)
  RouterProvider->>OptionalWrap: OptionalWrap(children)
  OptionalWrap->>RouterCtx: provide router context
  RouterCtx->>Matches: render routes/outlet
  Matches->>OptionalInner: OptionalInner(ResolvedSuspense)
  OptionalInner->>Suspense: render route content
  alt loader pending
    Suspense->>PendingFallback: render defaultPendingComponent (wrapped)
  else loaded
    Suspense->>RouteComponent: render route component (wrapped)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Assessment against linked issues

Objective Addressed Explanation
Ensure Wrap wraps routed components so their context is accessible (#3744)
Ensure InnerWrap wraps routed components so their context is accessible (#3744)
Ensure InnerWrap also wraps Suspense pending fallback/defaultPendingComponent (#3744)
Maintain public API without breaking changes (#3744)

Suggested reviewers

  • schiller-manuel

Poem

In burrows of code I hop and weave,
Wraps now cuddle routes like you’d believe.
InnerWrap hums soft, "You're safe within,"
Suspense waits patient—no panic, no din.
Hooray, contexts bloom where carrots begin! 🥕🐇

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 966534a and ee01696.

📒 Files selected for processing (2)
  • packages/solid-router/tests/Matches.test.tsx (2 hunks)
  • packages/solid-router/tests/RouterProvider.test.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/solid-router/tests/RouterProvider.test.tsx
  • packages/solid-router/tests/Matches.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Test
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbit in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbit in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbit gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbit read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbit help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbit ignore or @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbit summary or @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbit or @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@nx-cloud
Copy link

nx-cloud bot commented Aug 28, 2025

View your CI Pipeline Execution ↗ for commit ee01696

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 1m 50s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 3s View ↗

☁️ Nx Cloud last updated this comment at 2025-08-28 14:07:42 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Aug 28, 2025

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@5033

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@5033

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@5033

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@5033

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@5033

@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@5033

@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-ssr-query@5033

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@5033

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@5033

@tanstack/react-start-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-plugin@5033

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@5033

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@5033

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@5033

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@5033

@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@5033

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@5033

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@5033

@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-ssr-query-core@5033

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@5033

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@5033

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@5033

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@5033

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@5033

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@5033

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@5033

@tanstack/solid-start-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-plugin@5033

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@5033

@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@5033

@tanstack/start-plugin-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-plugin-core@5033

@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@5033

@tanstack/start-server-functions-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-functions-client@5033

@tanstack/start-server-functions-fetcher

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-functions-fetcher@5033

@tanstack/start-server-functions-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-functions-server@5033

@tanstack/start-storage-context

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-storage-context@5033

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@5033

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@5033

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@5033

commit: ee01696

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (7)
packages/solid-router/src/RouterProvider.tsx (1)

33-41: Avoid potential breaking change: let Wrap access router context; also tighten typing and prefer nullish coalescing.

Currently, Wrap is rendered outside the router context provider, which prevents Wrap from calling useRouter. Invert the order so Wrap still wraps all children (including pending fallbacks) while retaining access to router context. Also, explicitly type the wrapper to “accepts children” and use “??” over “||”.

-  const OptionalWrapper = router.options.Wrap || SafeFragment
+  const OptionalWrapper =
+    (router.options.Wrap ?? SafeFragment) as Solid.Component<{
+      children?: Solid.JSX.Element
+    }>

-  return (
-    <OptionalWrapper>
-      <routerContext.Provider value={router as AnyRouter}>
-        {children()}
-      </routerContext.Provider>
-    </OptionalWrapper>
-  )
+  return (
+    <routerContext.Provider value={router as AnyRouter}>
+      <OptionalWrapper>{children()}</OptionalWrapper>
+    </routerContext.Provider>
+  )

Action: Please confirm there are no known Wrap implementations relying on useRouter; if there are, the above change is necessary to avoid a regression. If not, consider documenting that Wrap must not read router context.

packages/solid-router/src/Matches.tsx (2)

47-62: InnerWrap placement is correct; small typing/coalescing tweak suggested.

Great fix—placing InnerWrap outside Suspense ensures defaultPendingComponent inherits the context. Suggest minor type and “??” coalescing adjustment for robustness.

-  const OptionalWrapper = router.options.InnerWrap || SafeFragment
+  const OptionalWrapper =
+    (router.options.InnerWrap ?? SafeFragment) as Solid.Component<{
+      children?: Solid.JSX.Element
+    }>

51-56: Optional rendering micro-tweak.

Using a JSX element for defaultPendingComponent is fine. If you want slightly clearer intent and avoid creating the element when undefined, Dynamic also works:

-        fallback={
-          router.options.defaultPendingComponent ? (
-            <router.options.defaultPendingComponent />
-          ) : null
-        }
+        fallback={
+          router.options.defaultPendingComponent ? (
+            <Solid.Dynamic component={router.options.defaultPendingComponent} />
+          ) : null
+        }
packages/solid-router/tests/RouterProvider.test.tsx (1)

1-37: Test clearly validates Wrap context propagation; tiny readability nit.

Consider declaring ctx before defining the component to reduce cognitive load when reading the closure.

-    const rootRoute = createRootRoute({
+    const ctx = createContext<string>()
+    const rootRoute = createRootRoute({
       component: () => {
         const contextValue = useContext(ctx)
         expect(contextValue, 'Context is not provided').not.toBeUndefined()

         return <div>{contextValue}</div>
       },
     })
@@
-    const ctx = createContext<string>()
-
     render(() => (
       <RouterProvider
         router={router}
         Wrap={(props) => {
           return <ctx.Provider value={'findMe'}>{props.children}</ctx.Provider>
         }}
       />
     ))
packages/solid-router/tests/Matches.test.tsx (3)

34-37: Remove stray console.log from tests.

This can add noise to CI output.

-  console.log('Matchse', matches())

129-161: Good coverage for InnerWrap → route components; rename local “screen” to avoid shadowing import.

Shadowing the imported screen with the render result is confusing.

-  const screen = render(() => (
+  const app = render(() => (
@@
-  const indexElem = await screen.findByText('context-for-children')
+  const indexElem = await app.findByText('context-for-children')

163-223: Good coverage for InnerWrap → defaultPendingComponent; avoid shadowing and consider fake timers.

  • Rename local screen as above.
  • Optional: use vi.useFakeTimers() and advanceTimersByTime(300) to reduce flakiness from real timers.
-  const screen = render(() => (
+  const app = render(() => (
@@
-  const linkToHome = await screen.findByRole('link', {
+  const linkToHome = await app.findByRole('link', {
@@
-  const indexElem = await screen.findByText('context-for-default-pending')
+  const indexElem = await app.findByText('context-for-default-pending')

If you want to switch to fake timers:

import { vi } from 'vitest'

// inside test
vi.useFakeTimers()
...
fireEvent.click(linkToHome)
vi.advanceTimersByTime(300)
...
vi.useRealTimers()
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a98c89c and 966534a.

📒 Files selected for processing (4)
  • packages/solid-router/src/Matches.tsx (2 hunks)
  • packages/solid-router/src/RouterProvider.tsx (2 hunks)
  • packages/solid-router/tests/Matches.test.tsx (2 hunks)
  • packages/solid-router/tests/RouterProvider.test.tsx (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/solid-router/src/Matches.tsx (2)
packages/solid-router/src/Transitioner.tsx (1)
  • Transitioner (11-140)
packages/solid-router/src/Match.tsx (1)
  • Match (22-155)
packages/solid-router/tests/RouterProvider.test.tsx (1)
packages/solid-router/src/RouterProvider.tsx (1)
  • RouterProvider (44-53)
packages/solid-router/tests/Matches.test.tsx (1)
packages/solid-router/src/RouterProvider.tsx (1)
  • RouterProvider (44-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Test
🔇 Additional comments (4)
packages/solid-router/src/RouterProvider.tsx (1)

3-3: LGTM: SafeFragment import is appropriate.

packages/solid-router/src/Matches.tsx (1)

77-80: LGTM: safer Match rendering.

The guard around matchId avoids non-null assertions and is clearer.

packages/solid-router/tests/Matches.test.tsx (2)

3-3: LGTM: imports for context usage.


8-9: createMemoryHistory re-export confirmed – The barrel at packages/solid-router/src/index.ts exports createMemoryHistory (line 203); tests can rely on it.

Copy link
Member

@birkskyum birkskyum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@birkskyum birkskyum merged commit 4b792b9 into TanStack:main Aug 29, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrap and InnerWrap not properly wrapping router components (@tanstack/solid-start)

2 participants